Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable recommended golang-ci linters #2204

Merged
merged 24 commits into from
Nov 4, 2024
Merged

Enable recommended golang-ci linters #2204

merged 24 commits into from
Nov 4, 2024

Conversation

AndrewSirenko
Copy link
Contributor

Is this a bug fix or adding new feature?
Refactor

What is this PR about? / Why do we need it?
We rely on golang-ci's suite of linters to analyze our code for potential errors or style issues. We have had most linters disabled for a while, instead relying on PR reviewers to catch common errors.

This PR re-enables golang-ci's linters, and refactors the code base to pass those linters.

Please see .golangci.yml for a list of enabled/disabled linters and individual commit messages for the code changes required to enable each of them.

It was requested that this be a single PR, though if it takes too long to review I will split it into two PRs:

  • one for zero-risk changes (style)
  • and one for slightly risky changes (refactoring)

What testing is done?
Manual testing, CI, make verify

@AndrewSirenko
Enable default golangci linters
bc0d1c0
@AndrewSirenko
Enable whitespace linter
a8e7848
@AndrewSirenko
cleanup: Delete copying of loop variables (Go 1.22+)
3d3840b
@AndrewSirenko
Enable dupword linter
3d3f9a0
@AndrewSirenko
cleanup: Ensure all variable names conform to 'ErrXxx' format
e239fd8
@AndrewSirenko
cleanup: Do not log with fmt.Println (forbidigo)
1dd1c11
@AndrewSirenko
Enable forcetypeassert linter
9dd6e71
@AndrewSirenko
cleanup: enable gci linter to make package import order deterministic
43d3f17
@AndrewSirenko
cleanup: enable gocritic linter and simplify elseIf chains
49aae91
@AndrewSirenko
cleanup: enable goimports linter
39efb4e
@AndrewSirenko
cleanup: Enable unconvert linter and remove unecessary type conversions
2ffd949
@AndrewSirenko
cleanup: change for loops to use integer range (Go 1.22+)
ef3bba5
@AndrewSirenko
cleanup: pre-allocate slices to increase performance
42f7b9d
@AndrewSirenko
cleanup: Use t.Setenv() in unit tests instead of os.Setenv
0a6d0c0
@AndrewSirenko
cleanup: Add t.Parallel() to more unit tests
735a034
@AndrewSirenko
cleanup: Rename vars with same name as a predeclared identifier
2673e24
@AndrewSirenko
Enable stylecheck linter; Fix ST1003 & ST1005
c18bef1
@AndrewSirenko
Ensure test helper functions start from t.Helper()
4b723c6
@AndrewSirenko
Enable goconst linter
1267d0a
@AndrewSirenko
Enable gochecknoinits linter
46c49c4
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 1, 2024
@AndrewSirenko AndrewSirenko changed the title Enable recommended golang-ci linters wip: Enable recommended golang-ci linters Nov 1, 2024
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 1, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 1, 2024

Code Coverage Diff

File Old Coverage New Coverage Delta
github.com/kubernetes-sigs/aws-ebs-csi-driver/cmd/hooks/prestop.go 81.1% 76.4% -4.8
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver/controller_modify_volume.go 78.3% 78.0% -0.3
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver/node.go 92.0% 92.2% 0.2

@AndrewSirenko AndrewSirenko changed the title wip: Enable recommended golang-ci linters Enable recommended golang-ci linters Nov 1, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 1, 2024
@AndrewSirenko AndrewSirenko changed the title Enable recommended golang-ci linters wip: Enable recommended golang-ci linters Nov 1, 2024
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 1, 2024
ConnorJC3
ConnorJC3 reviewed Nov 1, 2024
View reviewed changes
.golangci.yml Outdated Show resolved Hide resolved
.golangci.yml Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
@AndrewSirenko AndrewSirenko changed the title wip: Enable recommended golang-ci linters Enable recommended golang-ci linters Nov 1, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 1, 2024
torredil
torredil reviewed Nov 4, 2024
View reviewed changes
Copy link
Member

@torredil torredil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Largely lgtm 👍
Left a few non-blocking comments.

/approve

pkg/driver/controller.go Show resolved Hide resolved
pkg/cloud/volume_limits.go Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: torredil

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2024
ElijahQuinones
ElijahQuinones reviewed Nov 4, 2024
View reviewed changes
pkg/driver/controller.go Outdated Show resolved Hide resolved
pkg/driver/controller.go Show resolved Hide resolved
ConnorJC3
ConnorJC3 reviewed Nov 4, 2024
View reviewed changes
Copy link
Contributor

@ConnorJC3 ConnorJC3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm (other than needing a squash - please don't approve until those fixup commits are squashed)

ConnorJC3
ConnorJC3 approved these changes Nov 4, 2024
View reviewed changes
Copy link
Contributor

@ConnorJC3 ConnorJC3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2024
@k8s-ci-robot k8s-ci-robot merged commit 7f8e841 into kubernetes-sigs:master Nov 4, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElijahQuinones ElijahQuinones ElijahQuinones left review comments

@torredil torredil torredil left review comments

@ConnorJC3 ConnorJC3 ConnorJC3 approved these changes

Assignees

@ConnorJC3 ConnorJC3

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AndrewSirenko @k8s-ci-robot @ConnorJC3 @torredil @ElijahQuinones